Break tie for top categorical columns in Series.describe
#9867
Conversation
isVoid
added
3 - Ready for Review
galipremsagar
added
5 - Ready to Merge
|
@isVoid Looks like we need to fix the style of a file |
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #9867 +/- ##
================================================
- Coverage 10.49% 10.42% -0.07%
================================================
Files 119 119
Lines 20305 20473 +168
================================================
+ Hits 2130 2134 +4
- Misses 18175 18339 +164
Continue to review full report at Codecov.
|
|
@bdice is this good to go? Looks like the review has been addressed |
| dtype="str", | ||
| index=index, | ||
| index=data.keys(), | ||
| nan_as_null=False, | ||
| name=self.name, | ||
| ) | ||
|
|
||
| def _describe_timestamp(self): |
There was a problem hiding this comment.
The describe implementations appear to be casting all the values to str (aside from the numeric implementation). This does not align with Pandas behavior:
>>> s = pd.Series([
... np.datetime64("2000-01-01"),
... np.datetime64("2010-01-01"),
... np.datetime64("2010-01-01"),
... ])
>>> print(type(s.describe()["top"]))
<class 'pandas._libs.tslibs.timestamps.Timestamp'>I recognize there is an issue here with different types, namely that count and freq are not of the same type as mean, min, percentiles, or max.
Some options to resolve this (and their downsides):
- Current implementation: return all values as
str(results are on GPU ...but data is not usable asstrtype). - Return a
pd.DataFrameordictthat can have multiple types (not a GPU DataFrame).
Personally I think option (2) is the better choice here. The summary doesn't really need to be a GPU DataFrame since it contains so few values. Do we have precedent for this kind of behavior returning a CPU (Pandas) DataFrame?
I'm content to resolve this with the current implementation as well. Just wanted to note the inconsistency. I can file an issue if needed.
There was a problem hiding this comment.
I agree that a copy to host that preserves more type information is probably preferable. I'm not aware of any API that returns a pandas DataFrame, but we do return pandas objects in some places. One example is DataFrame.columns, which returns a pandas Index, probably based on the same consideration of not wanting to put the data on device. Personally from an API design perspective I would prefer not to return pandas objects and would rather return something like a dict, but I can see that causing more friction for users switching between pandas and cudf.
IMO this change is out of scope for this PR in any case. We should open an issue to discuss this further and make a more holistic discussion, but we shouldn't hold up progress here.
Co-authored-by: Bradley Dice <[email protected]>
|
@gpucibot merge |
Closes #9825
This PR fixes the bug that when there are ties with most frequent categories in categorical series, the order returned is undefined. We break the tie by sorting the category and take its top.
Also included in this PR: refactors
describeto use a dictionary items to describe for better readability. And enablescudf/python/cudf/cudf/tests/test_series.py
Line 405 in 024003c